Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

txnbuild: adds a 5 minute grace period to ReadChallengeTx MinTime constraint #3824

Conversation

JakeUrban
Copy link
Contributor

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Adds a 5 minute grace period to ReadChallengeTx's Transaction.Timebounds().MinTime constraint to account for clock drift between the client and server.

Why

The JS SDK recently merged a similar change (commit), which is now requested for the Go SDK (issue).

Known limitations

N/A

@JakeUrban JakeUrban linked an issue Aug 12, 2021 that may be closed by this pull request
@JakeUrban JakeUrban force-pushed the txnbuild-add-grace-period-to-challenge-reader branch 2 times, most recently from c5be599 to 4de8287 Compare August 12, 2021 21:19
@JakeUrban
Copy link
Contributor Author

I had originally created my feature branch on master and force-pushed after rebasing on the 8.0 release branch.

@JakeUrban
Copy link
Contributor Author

@ire-and-curses I'm not sure who should review this so feel free to add someone and remove yourself 👍

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good. 👏🏻 We could also make it configurable, but there's no rush to do that now and we can always expose the config later if needed. There's value in making this configurable in SDKs that are used with clients and servers, but less so with servers.

@leighmcculloch
Copy link
Member

@JakeUrban This change is not breaking and you could have it target master, unless this needs to be in a major release for some reason not mentioned here.

@JakeUrban JakeUrban force-pushed the txnbuild-add-grace-period-to-challenge-reader branch from bfd1043 to 5b2b40b Compare September 15, 2021 21:19
@JakeUrban JakeUrban changed the base branch from release-txnbuild-v8.0.0 to master September 15, 2021 21:19
@JakeUrban
Copy link
Contributor Author

I force pushed to rebase on master, no changes to the code though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEP-10: Accept challenge transactions through clock drift
3 participants